Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Any asset fee feature #10

Merged
merged 81 commits into from
Apr 13, 2024
Merged

Any asset fee feature #10

merged 81 commits into from
Apr 13, 2024

Conversation

Mixa84
Copy link
Collaborator

@Mixa84 Mixa84 commented Apr 9, 2024

This covers the first milestone of the No Coin project, modifying the on-chain protocol to support fee payment in any asset. It also includes the necessary changes to the mempool to allow block signers to valuate assets at their desired exchange rates, and introduces two new exchangerate RPCs for getting and setting those rates.

Cliff notes:

  • Adds global Sequentia flag g_con_any_asset_fees for enabling No Coin feature
  • Support fee payment with any asset (via createrawtransaction, fundrawtransaction, and sendtoaddress)
  • Default to asset used in transaction when selecting fee asset in coin selection
  • Reward transaction fees in any asset in coinbase payout to fee wallet
  • Add RPCs for getting and setting asset exchange rates (getfeeexchangerates and setfeeexchangerates)
  • Alternatively, allow exchange rates to be set using a JSON config file at startup
  • Recompute fee values in mempool entries whenever exchange rates are updated
  • Update transaction RPCs to include information about fee asset (gettransaction and listtransactions)

JBetz added 29 commits April 11, 2024 19:49
@Mixa84 Mixa84 changed the title "No coin" feature Any asset fee feature Apr 12, 2024
Copy link

@fare fare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, but a few minor changes needed.

src/exchangerates.cpp Outdated Show resolved Hide resolved
src/exchangerates.h Outdated Show resolved Hide resolved
src/exchangerates.h Outdated Show resolved Hide resolved
src/node/miner.cpp Outdated Show resolved Hide resolved
src/node/miner.cpp Outdated Show resolved Hide resolved
void ClearPrioritisation(const uint256& hash);

/** Recompute valuation of all transaction fees, called whenever exchange rates have been updated. */
void RecomputeFees();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need that EXCLUSIVE_LOCKS_REQUIRED here? Why or why not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecomputeFees() takes the lock, and EXCLUSIVE_LOCKS_REQUIRED means that the lock must already be held. So we don't need it here.


CAsset feeAsset;
if (g_con_any_asset_fees) {
feeAsset = fee_map.begin()->first;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there's zero, or two or more?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, at least explain it in a comment.

Copy link
Contributor

@JBetz JBetz Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment and some TODOs in: 65ffeb0

The short answer is that there should never be more than one, and since ConstructTransaction enforces this, it's loosely correct by construction. If we want a stronger guarantee, we might want to add a check in consensus.

src/validation.cpp Outdated Show resolved Hide resolved
src/wallet/coincontrol.h Outdated Show resolved Hide resolved
self.node1_nonct_address = self.nodes[1].getaddressinfo(self.node1_address)["unconfidential"]

self.nodes[0].setfeeexchangerates({ "gasset": 100000000, self.asset: 100000000 })
self.nodes[1].setfeeexchangerates({ "gasset": 100000000, self.asset: 100000000 })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include test of getfeeexchangerates

@JBetz JBetz requested a review from fare April 12, 2024 15:00
}
EnsureAnyMemPool(request.context).RecomputeFees();
return NullUniValue;
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is good enough for now, but then open a followup issue so that:

  1. the load-from-file and load-from-rpc share the same code
  2. that code validates the entire data structure before it is used, so the global exchangeRateMap is not left in unusable state if something goes wrong.
  3. maybe save the json to a temporary file, fdatasync, then atomically replace the startup file, if things go right.

@fare fare merged commit 659c80f into dev Apr 13, 2024
1 check passed
@fare fare deleted the no-coin branch April 13, 2024 00:52
@JBetz JBetz removed their assignment Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants